Skip to content

Add RBS type signatures#278

Closed
sferik wants to merge 6 commits intoruby:masterfrom
sferik:add-rbs-type-signatures
Closed

Add RBS type signatures#278
sferik wants to merge 6 commits intoruby:masterfrom
sferik:add-rbs-type-signatures

Conversation

@sferik
Copy link

@sferik sferik commented Mar 4, 2026

No description provided.

@hsbt
Copy link
Member

hsbt commented Mar 4, 2026

@soutaro @pocke @ksss Could you review this? I consider adding test task for this like same as bundled gems. But I'm not sure what's the current direction of rbs with default gems.

@sferik
Copy link
Author

sferik commented Mar 5, 2026

@hsbt I'm working on a commit to add type checking/verification with Steep now.

@sferik sferik force-pushed the add-rbs-type-signatures branch from 47d857d to 06efd91 Compare March 5, 2026 01:01
@ksss
Copy link

ksss commented Mar 5, 2026

@sferik
If you wish to enrich the type definitions for net/http, please send a PR to the ruby/rbs repository.
Currently, we manage type definitions in the ruby/rbs repository, but it would also be ideal to manage them within the gem itself.
If you wish to move the net/http type definitions to the gem itself for management, attention should be paid to the coverage of the type definitions.

Which one would you like?

@sferik
Copy link
Author

sferik commented Mar 5, 2026

@ksss
I think it makes the most sense to manage the type definitions in this repo. Colocating type definitions with the source code will make it easier to keep them in sync. When a contributor adds a new public method, they can update the code, tests, and type signatures all in a single pull request. If the type definitions live in a separate repository, adding a new public method would require two PRs to two separate repos.

I've added Steep as a development dependency along with a new rake task to validate the RBS file against the code, as well as a GitHub workflow that will type check new PRs.

I understand the concern about type coverage. The signatures in this PR were generated with typeprof and rbs prototype rb and then refined by hand. I'd be happy to improve coverage further if there are specific gaps you'd like addressed.

@sferik sferik force-pushed the add-rbs-type-signatures branch 3 times, most recently from 32765fb to 1c2cf68 Compare March 5, 2026 03:03
Copy link

@ksss ksss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with the argument that type definitions should be included in gems. In that case, the following should be considered:

@sferik
Copy link
Author

sferik commented Mar 5, 2026

Thank you for this valuable feedback. I am working on fixes now.

sferik added 2 commits March 5, 2026 11:27
Add `steep` to Gemfile, add `steep` rake task, and add `typecheck`
GitHub workflow.
@sferik sferik force-pushed the add-rbs-type-signatures branch 3 times, most recently from 328ba6a to d114582 Compare March 5, 2026 21:18
@sferik
Copy link
Author

sferik commented Mar 5, 2026

@ksss
I was not aware of the existing net-http.rbs in the rbs repo, so I started over by moving that file (and the manifest.yaml) into the sig/ directory.

I then added steep to the Gemfile, a steep task to the Rakefile, and a typecheck.yml GitHub action to run rake steep in CI.

I then ran rake steep locally and systematically fixed the type errors. This required some changes to the lib files (generally making the code more explicit/less dynamic) and some changes to the type signatures in sig/net-http.rbs. In cases where I needed to add type signatures for "internal use only" classes or other libraries, I added separate sig/*-impl.rbs files, which I have excluded from the gemspec. I was able to make rake steep pass without adding any # steep:ignore comments.

Finally, I added local RBS interface tests. Do you think I should also keep Steep or remove it?

Please let me know if I got anything wrong or missed anything. Thanks again for your feedback!

@sferik sferik force-pushed the add-rbs-type-signatures branch from d114582 to 5a7d2e1 Compare March 5, 2026 21:28
@sferik
Copy link
Author

sferik commented Mar 5, 2026

I also added rbs:test and rbs:annotate rake tasks, mirroring the ones in the bigdecimal Rakefile. I ran rbs:annotate and committed the changes to sync the RDoc documentation to the RBS file.

Please also let me know if you'd like me to squash these down into a single commit.

sferik added 2 commits March 5, 2026 13:52
Adjust library code to make dynamic behavior explicit for Steep with
small, behavior-preserving changes: add local narrowing, nil guards, key
coercions, read_body call shaping, and a few compatibility-focused
internal fixes.
Update the public RBS to better match actual net/http behavior around
URI handling, nilable headers and response bodies, exception classes,
and request/response entry points.

Keep shipped signatures in sig/ focused on net/http’s public API, and
add a separate *-impl.rbs files for implementation-only declarations
used only by Steep when typechecking lib/.

The *-impl.rbs files cover internal protocol classes, helper methods,
constants, and stdlib gaps needed by the implementation, without
shipping those internal details as part of the gem’s public RBS.
@sferik sferik force-pushed the add-rbs-type-signatures branch 3 times, most recently from 174b9b2 to 8527e9c Compare March 5, 2026 22:54
@sferik sferik force-pushed the add-rbs-type-signatures branch from 8527e9c to 9b7151a Compare March 5, 2026 23:31
Copy link

@ksss ksss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no maintenance privileges whatsoever for this repository. I don't even have the authority to approve this PR. I will state my personal opinion based on this premise.

I think simply copying net-http.rbs was an excellent decision. That's where you should start. However, you should start with as complete a copy as possible. I don't know what the differences are, so I can't review it.

I think we should first implement the introduction of RBS, and then as the next step, tackle changes to the Ruby code. These seem like separate issues. I think the PRs should be split.

Steep is a development support tool. If you want to use Steep for developing this repository, then I think it should be introduced, but otherwise, I think it should be left to the developers.

The idea of *-impl.rbs is great, but for the same reason as Steep, I don't think it should be included in this PR.

@sferik
Copy link
Author

sferik commented Mar 6, 2026

Closing this PR in favor of:

Feel free to review and merge each of these independently. I hope this helps move this issue forward.

@sferik sferik closed this Mar 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants